Add a C module for handling buttons. by deshipu · Pull Request #295 · adafruit/circuitpython

deshipu · 2017-09-29T20:26:43Z

Fix #279
This adds a buttons module to the samd port, with two functions useful for button handling: setup() and get_pressed(). The buttons are automatically scanned every 32 ticks, de-bounced, and reported to the user as a bit mask.

tannewt · 2017-09-29T22:53:35Z

In addition to adding the description from the issue here is more feedback.

What do you think about renaming this to gamepad? I'm worried that calling it buttons implies that you need it whenever using a button.

I also think it should be a proper object rather than using statically allocated memory. That gives more flexibility on its use going forwards without impacting the size of a global state. The only global state should be the object mechanics (functions and name dictionaries) plus a global for a pointer to a Gamepad object that tick updates.

Please add this to the top-level shared-bindings too. I think its something folks should add to other ports as needed. Document its availability here: https://github.com/adafruit/circuitpython/blob/master/shared-bindings/index.rst

Thanks! Your description on the issue made this click for me and now I'm excited about it.

deshipu · 2017-09-29T23:08:51Z

I also think it should be a proper object rather than using statically allocated memory. That gives more flexibility on its use going forwards without impacting the size of a global state. The only global state should be the object mechanics (functions and name dictionaries) plus a global for a pointer to a Gamepad object that tick updates.

Let me see if I understand this correctly. I would have a global (extern) pointer to a GamePad object, initialized to NULL, that would be checked in tick.c and a method on it called if it's not NULL, and then in gamepad.setup() I would create that object (if it doesn't exist yet), and assign it to that global pointer?

No, sorry, it doesn't make sense, C doesn't have methods, so tick.c would still call a regular function, which in turn would check that global variable for its state.

tannewt · 2017-09-29T23:13:48Z

I think we're close to the same page. gamepad.GamePad(b1, b2) would construct a python object and set the global state to it. I was thinking tick would then see if the gamepad global is null but its probably better to have it always call something in gamepad which can do a null check internally.

Although C doesn't have methods, its common for CircuitPython code to pass around a struct representing an object as the first argument similar to how Python passes self around.

deshipu · 2017-09-29T23:26:43Z

Ah, so instead of setup() you want a class. But then what happens when you create two such objects? With setup(), you simply replace the previous configuration, but doing that with class constructor would be surprising.

tannewt · 2017-09-29T23:27:43Z

Yeah, you could either throw an exception or have the underlying structs be a linked list that can be updated.

deshipu · 2017-09-29T23:29:33Z

Then we also need a way to remove buttons, and to deinitialize the whole thing, and it becomes much more complicated.

deshipu · 2017-09-29T23:32:04Z

I will try to use an object internally, but leave the API as it is for now, we can test it and think about a better API then.

tannewt · 2017-09-29T23:35:58Z

Why do you need a way to remove buttons?

Needing to deinit is a common problem in the existing APIs. They do it through __exit__, __deinit__ and an internal reset that can reset the global to null.

deshipu · 2017-09-29T23:47:23Z

It seems natural that if you can enable something, you should also be able to disable it.

One use case that comes to mind is a common trick with sharing buttons with the SPI pins — the buttons are connected through resistors, so that when the CS pin is asserted, the pins are in output mode and SPI has control over the lines, but when we finish and deassert CS, we switch them back into input and read the button states from them. It's a bit of a hack, but surprisingly common, and it would be useful on boards like Trinket M0, where the SPI uses up almost all pins, but you probably would still want to have some buttons with that screen.

deshipu · 2017-09-30T00:20:53Z

I tried it with a dynamically allocated object, but the extra code made it not fit on the Trinket anymore. Is there an important reason to avoid those 8 statically allocated bytes, or is this just a question of style?

tannewt · 2017-09-30T01:14:47Z

Could you push that version anyway? Its mostly style because static storage is always used vs the heap which is dynamic.

deshipu · 2017-09-30T10:27:58Z

I will try again, but I really wonder if it's worth it to fight for those 8 bytes, especially sice the global pointer to the object alone will take 4 bytes already, not even counting all the extra code that is needed.

(I just realized that it's an array of pointers, not of pin numbers, so it's actually 32 bytes.)

deshipu · 2017-09-30T23:30:46Z

I re-did the GamePad object, this time I simply made it a singleton object — trying to create more than one simply re-configures the existing one. I still don't like the fact that you have to keep the reference to the created object and pass it around your program.

tannewt

Thanks for a rev on this! I realize I'm being picky but I want to make sure core APIs are consistent before adding them.

You've convinced me this is useful so I'd love it if you'd see it through.

Regarding passing it around as state, that would make things simpler for small things but global state can make things complicated. I'd like to draw the boundary between state that needs to kept around and global state by acknowledging what state persists across VM runs and what doesn't. Files and autoreload are examples of state that persists across VMs while this and all of the other hardware interactions get reset after a VM ends.

I hope that makes sense. I'd love to hear about other changes you are making for pew pew because we could have it in this repo as an experimental build.

tannewt · 2017-10-03T00:35:31Z

+//|   :platform: SAMD21
+//|
+
+//| ..method:: GamePad.get_pressed()


Move this below the class, then indent it and it'll be included in the class automatically. Here is an example: https://github.com/adafruit/circuitpython/blob/master/shared-bindings/touchio/TouchIn.c#L54

I also think pressed should be a property rather than a method because its state.

I also think pressed should be a property rather than a method because its state.

I would make it a property, if not for the side effect, which is clearing the current state. The way this works, it accumulates button presses, until you call "get_pressed", which gives you what has been pressed since the last call, and clears the buffer. Making it a property would lead to surprising, unexpected behaviors where the status is cleared more than once, etc.

I made the status clearing functionality more explicit in the description.

Also note, that it is important that the status is retrieved and cleared in one atomic operation, and not first retrieved, and then cleared separately — the latter risks missing some button presses.

Ok! Looks good.

Another critical point in my mind is that it includes past presses. The state module works better when there are no side effects and it only is actively pressed buttons. Thanks for the push back!

tannewt · 2017-10-03T00:39:16Z

+    .name = MP_QSTR_GamePad,
+    .make_new = gamepad_make_new,
+    .locals_dict = (mp_obj_dict_t*)&gamepad_locals_dict,
+};


Please move the GamePad object and header to a separate file to match TouchIn and others. https://github.com/adafruit/circuitpython/blob/master/shared-bindings/touchio/TouchIn.c#L54

tannewt · 2017-10-03T00:40:25Z

+//| immediately get switched to input with a pull-up, and then scanned
+//| regularly for button presses. The order is the same as the order of
+//| bits returned by the ``get_pressed`` function. To disable button
+//| scanning, call this without any arguments.


Disabling this by calling it again with no arguments seems weird to me. Could you add deinit to match other classes instead? Thanks!

deshipu · 2017-10-03T08:18:04Z

Thank you for your guidance, I'm still not very well oriented in how it all is organized, your comments are certainly most helpful.

I'm still not sure what to do with the conflicts. I originally wrote this for the 2.x version, and it seems that in 3.x there has been some changes — to which version do we want to merge this?

tannewt · 2017-10-03T18:30:59Z

+
+//| ..class:: GamePad([b1[, b2[, b3[, b4[, b5[, b6[, b7[, b8]]]]]]]])
+//|
+//| Initializes button scanning routines.


Have you tested this with sphinx or readthedocs? I think this chunk and below needs to be indented.

(RST was created by python people I think so its white space sensitive too.)

I haven't, I will fix it and test, sorry.

tannewt · 2017-10-03T18:32:36Z

+//| .. module:: gamepad
+//|   :synopsis: Button handling
+//|   :platform: SAMD21
+//|


35-41 can go in init.c and do .. currentmodule:: gamepad here.

Example touchio docs: https://github.com/adafruit/circuitpython/blob/master/shared-bindings/touchio/__init__.c

I will try that, thanks.

tannewt · 2017-10-03T18:35:37Z

 #define CIRCUITPY_AUTORELOAD_DELAY_MS 500
 #define CIRCUITPY_BOOT_OUTPUT_FILE "/boot_out.txt"
+// Scan gamepad every 32ms
+#define CIRCUITPY_GAMEPAD_TICKS 0x1f


This should be in the express board portion right? Currently this will call ticks but the singleton will always be null.

Not sure what you mean. This defines how often the buttons are scanned, and also lets you disable the scanning completely. I put it here, because it's configuration. The gamepad_singleton is not related to it, but it won't be always NULL — it's being assigned to in the gamead_make_new function.

Right, the scanning should be disabled on non-express builds. With this here, non-express boards will still include https://github.com/adafruit/circuitpython/pull/295/files#diff-ae7239759d3115ddffbb5b5fd105f23aR21 and gamepad_ticks().

Ah, I see what you mean.

tannewt · 2017-10-03T18:37:16Z

+//|   :platform: SAMD21
+//|
+
+//| ..method:: GamePad.get_pressed()


Ok! Looks good.

Another critical point in my mind is that it includes past presses. The state module works better when there are no side effects and it only is actively pressed buttons. Thanks for the push back!

tannewt

A few minor doc things.

Pulling this into 2.x as you've tested is fine. I want to move people to master quickly but I've just done it a bit prematurely. I'll make sure everything in 2.x makes it into master.

tannewt · 2017-10-03T18:39:45Z

That said, you should be able to test this in master with the REPL if you want to try and merge to master. The filesystem stuff isn't working yet though.

deshipu · 2017-10-03T20:01:53Z

Thank you for the review, I incorporated your suggestions, and also added a short usage example.

tannewt

Great! Which branch do you want to merge it into?

deshipu · 2017-10-03T20:16:10Z

I tested it with 2.x, so I think it would make sense to merge it there. Should I make a second pull request for that branch, or can you choose it while merging?

deshipu added 5 commits September 29, 2017 01:56

Initial proof of concept of the buttons module

281d73f

Fix the behavior of the get_buttons and setup_buttons

4d18b2f

Support up to 8 buttons

99e3acf

Move buttons to a separate module

1f5e0fb

Add documentation for the buttons module

1ca3685

Rename buttons to gamepad and move it to shared bindings

958875d

Finish gamepad rename and make gamepad ticks configurable

12fc5e9

deshipu added 3 commits September 30, 2017 18:19

Add gamepad module to the support matrix

c53ed5f

Move gamepad to extra building modules

624ecb8

Make a GamePad object in gamepad module

cb41776

tannewt self-requested a review October 3, 2017 00:41

tannewt requested changes Oct 3, 2017

View reviewed changes

Reorganize gamepad code

3701139

tannewt reviewed Oct 3, 2017

View reviewed changes

tannewt requested changes Oct 3, 2017

View reviewed changes

deshipu added 2 commits October 3, 2017 21:57

Update docs for gamepad module

1f178f5

Only call gamepad_tick when the module is enabled

97fab02

tannewt approved these changes Oct 3, 2017

View reviewed changes

tannewt changed the base branch from master to 2.x October 3, 2017 20:34

tannewt merged commit f498167 into adafruit:2.x Oct 3, 2017

deshipu mentioned this pull request Mar 10, 2021

RP2040: hangs when using gamepad #4166

Closed

Conversation

deshipu commented Sep 29, 2017

Uh oh!

tannewt commented Sep 29, 2017

Uh oh!

deshipu commented Sep 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tannewt commented Sep 29, 2017

Uh oh!

deshipu commented Sep 29, 2017

Uh oh!

tannewt commented Sep 29, 2017

Uh oh!

deshipu commented Sep 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deshipu commented Sep 29, 2017

Uh oh!

tannewt commented Sep 29, 2017

Uh oh!

deshipu commented Sep 29, 2017

Uh oh!

deshipu commented Sep 30, 2017

Uh oh!

tannewt commented Sep 30, 2017

Uh oh!

deshipu commented Sep 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deshipu commented Sep 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tannewt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deshipu commented Oct 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tannewt left a comment

Choose a reason for hiding this comment

Uh oh!

tannewt commented Oct 3, 2017

Uh oh!

deshipu commented Oct 3, 2017

Uh oh!

deshipu commented Sep 29, 2017 •

edited

Loading

deshipu commented Sep 29, 2017 •

edited

Loading

deshipu commented Sep 30, 2017 •

edited

Loading

deshipu commented Sep 30, 2017 •

edited

Loading